Skip to content

Conversation

jyn514
Copy link
Member

@jyn514 jyn514 commented Dec 12, 2020

Fixes an assertion failure when resolving ::std (or any other crate that uses the :: style, see https://github.com/rust-lang/rust/pull/79809/files#r541776478, https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Perf.20failing).

Unblocks rust-lang/rustc-perf#806.

r? @ghost - this is a trivial fix and breaking rustc-perf so I'm going to approve it unilaterally. cc @Mark-Simulacrum @Eric-Arellano

@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name labels Dec 12, 2020
@jyn514
Copy link
Member Author

jyn514 commented Dec 12, 2020

@bors r+ p=1

This is breaking rustc-perf.

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

📌 Commit 130dbe4 has been approved by jyn514

@bors bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Dec 12, 2020
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the hotfix!

@@ -436,8 +436,8 @@ impl<'a, 'tcx> LinkCollector<'a, 'tcx> {
// Try looking for methods and associated items.
let mut split = path_str.rsplitn(2, "::");
// NB: `split`'s first element is always defined, even if the delimiter was not present.
// NB: `item_str` could be empty when resolving in the root namespace (e.g. `::std`).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh wait, I'm confused. Note that rsplit returns a reverse iterator. In this example, I think the first element would be std, right? It would not be empty.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the debug output from the assertion failure on hyper:

TRACE rustdoc::passes::collect_intra_doc_links considering link '::client'
DEBUG rustdoc::passes::collect_intra_doc_links resolving ::client as a macro in the module DefId(0:248 ~ hyper[8787]::body)
DEBUG rustdoc::passes::collect_intra_doc_links ::client resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links ::client resolved to Err(()) in namespace ValueNS
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
DEBUG rustdoc::passes::collect_intra_doc_links looking for enum variant ::client
DEBUG rustdoc::passes::collect_intra_doc_links  resolved to Err(()) in namespace TypeNS
thread 'rustc' panicked at 'assertion failed: !item_str.is_empty()', src/librustdoc/passes/collect_intra_doc_links.rs:439:9

So I think what's going wrong is that resolve_variant calls resolve recursively, and on the second call the string is empty (and only has one item in the iterator). This could probably be improved but it would require a redesign of the algorithm.

Copy link
Member Author

@jyn514 jyn514 Dec 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, this is the backtrace:

   0: std::panicking::begin_panic
             at /home/joshua/rustc/library/std/src/panicking.rs:519:12
   1: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:439:9
   2: rustdoc::passes::collect_intra_doc_links::LinkCollector::check_full_res
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:637:17
   3: rustdoc::passes::collect_intra_doc_links::resolution_failure::{{closure}}
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1680:33
   4: rustdoc::passes::collect_intra_doc_links::report_diagnostic::{{closure}}
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1597:9
   5: core::ops::function::FnOnce::call_once{{vtable.shim}}
             at /home/joshua/rustc/library/core/src/ops/function.rs:227:5
   6: <alloc::boxed::Box<F,A> as core::ops::function::FnOnce<Args>>::call_once
             at /home/joshua/rustc/library/alloc/src/boxed.rs:1328:9
   7: rustc_middle::lint::struct_lint_level::struct_lint_level_impl
             at /home/joshua/rustc/compiler/rustc_middle/src/lint.rs:362:9
   8: rustc_middle::lint::struct_lint_level
             at /home/joshua/rustc/compiler/rustc_middle/src/lint.rs:364:5
   9: rustc_middle::ty::context::TyCtxt::struct_span_lint_hir
             at /home/joshua/rustc/compiler/rustc_middle/src/ty/context.rs:2562:9
  10: rustdoc::passes::collect_intra_doc_links::report_diagnostic
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1567:5
  11: rustdoc::passes::collect_intra_doc_links::resolution_failure
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1617:5
  12: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve_with_disambiguator
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1299:21
  13: rustdoc::passes::collect_intra_doc_links::LinkCollector::resolve_link
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:1082:39
  14: <rustdoc::passes::collect_intra_doc_links::LinkCollector as rustdoc::fold::DocFolder>::fold_item
             at /home/joshua/rustc/src/librustdoc/passes/collect_intra_doc_links.rs:909:28

Notice this is in diagnostics, not the main resole code.

@bors
Copy link
Collaborator

bors commented Dec 12, 2020

⌛ Testing commit 130dbe4 with merge f61e5ca...

@bors
Copy link
Collaborator

bors commented Dec 13, 2020

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing f61e5ca to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Dec 13, 2020
@bors bors merged commit f61e5ca into rust-lang:master Dec 13, 2020
@rustbot rustbot added this to the 1.50.0 milestone Dec 13, 2020
@jyn514 jyn514 deleted the assertion-failure branch December 13, 2020 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-intra-doc-links Area: Intra-doc links, the ability to link to items in docs by name merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants